-
Notifications
You must be signed in to change notification settings - Fork 673
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New variant API #823
New variant API #823
Conversation
React devtools and Emotion will now render the correct component names
Thanks! There's some interesting ideas here -- I need to add the v1 roadmap issue I've been drafting, but some of this could potentially fit in line with some other parts of that |
@@ -18,7 +23,7 @@ export const AspectRatio = React.forwardRef( | |||
/> | |||
<Box | |||
{...props} | |||
__css={{ | |||
sx={{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be considering changing the private API a bit here, but does this mean that props.sx
is no longer forwarded to the component here? This looks like it'd break in MDX usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes you're right, sorry had overlooked that, I've re-added the __css
API.
packages/components/src/Badge.js
Outdated
{ variant, ...props }, | ||
ref | ||
) { | ||
const variation = useVariant('badges', variant) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This word/meaning bothers me, sorry...
const variation = useVariant('badges', variant) | |
const variantStyle = useVariant('badges', variant) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all good!
Here's an example my latest thoughts on a unified method to this whole approach: const modifiers = {
// Considering adding a special `default` key to automatically apply and allow overwriting sets of default styles:
default: {
title: { ... },
subtitle: { ... },
},
variant: {
primary: {
title: { ... },
subtitle: { ... },
}
},
size: {
small: {
title: { fontSize: 3 },
subtitle: { fontSize: 0 },
},
large: {
title: { fontSize: 5 },
subtitle: { fontSize: 2 },
},
},
// etc
};
const Component = ({ variant: 'primary', size = 'small', modifiers: customModifiers }) => {
const modifierStyle = useModifiers(
{ variant, size },
modifiers,
customModifiers // Optional secondary modifiers object that will merge with `modifiers`.
);
return (
<div>
<div sx={[{ fontFamily: 'heading' }, modifierStyle.title]}>{title}</div>
<div sx={modifierStyle.subtitle}>{subtitle}</div>
</div>
);
}; Some example usage: // Normal usage:
<Component size="large" /> // Overwriting some default styles:
<Component modifiers={{ default: { title: { ... }, subtitle: { ... } } }} /> // MyComponent.js
// Custom sizes:
const modifiers = {
small: {
title: { fontSize: 4 },
subtitle: { fontSize: 1 },
},
large: {
title: { fontSize: 6 },
subtitle: { fontSize: 2 },
},
};
const MyComponent = (props) => <Component {...props} modifiers={modifiers} />; General ideas: Much of these changes have come from ideas presented in #586.
I'd really like to get some discussion going to fine tune it. It's been a great API to work with so far in my own experience. I don't know if I am stepping outside of the core ideas behind theme-ui, but this solves a bunch of problems inherent with the current approach. One more example illustrating supplying custom styles to const MySelect = (props) => <Select {...props} modifiers={{ container: { ... }, select: { ... }, arrow: { ... } }}>...</Select>; |
This is purely a note from a conversation I had in private with @jxnblk for documentation sake: Coming from #990 I tested out the changes to the |
Hey @dburles sorry for the lack of activity/discussion here, but really appreciate the thought and effort that went into this! I think to help move our TS conversion along, we'd like to ditch the use of the |
@jxnblk No problem. There is definitely some more discussion needed with these changes. There are a bunch of scattered conversations on the variant API in general, what are your thoughts on creating a new banner issue to bring the discussion together as it's a little scattered right now. #800 is the main issue for this PR, there's also PR's #294 and #1017. Just to confirm, would you like me to create a new PR from these changes, but without the variant alterations? Let me know if there's anything I've missed. I'm not sure where that leaves this PR for the future, but I can move some of my comments from here over to any new discussion on the variant API changes in general. |
Yeah, a new issue or a few new issues might make sense. I think it should be generally inline with #832 or a proposal for a new API that can be evaluated on its own. e.g.
No, no, no. Sorry if I wasn't clear. I can create a new branch from this PR, descope a couple things and tidy it up for a new PR which would supersede this one, but keeping your commit history -- just wanted to make sure you were cool with that.
Starting from from a new issue is fine IMO, linking to prior art or past issues and PRs where needed. |
Sounds good to me |
Closing because we’re not planning on implementing this, & https://github.com/dburles/mystical is a successor if you’re looking for this functionality :) Thank you so much for the thoughtful PR though—we really appreciate it. |
This is a work in progress and a bit of a proof of concept refactor to replace the
variant
sx
property with thevariant
prop. See #800. This would be a breaking change, but theme-ui is not v1.0 yet 🔨😄. I've also refactored theBox
andFlex
components and removed the dependencies on bothstyled-system
and@emotion/styled
, which also addresses issues with #817 and #690. We could probably drop the usage ofBox
from all the other components now as it's not doing all that much. I've also decided to drop the use of the few styled-system props that were previously part of theBox
component in favour of using thesx
prop instead.There is also a new API for components with multiple elements:
Select
,Checkbox
andRadio
. It's now possible to supply styles to each part. Such as forSelect
:Todo:
css
functionuseVariant
hook public? – could be quite useful for custom components.